Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add touchenter event, add JSDoc, small refactor #4819

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kungfooman
Copy link
Collaborator

Fixes #4755

Firing mousemove even when the mouse is outside the ElementComponent is actually correct behaviour and other elements depend on this, e.g. ScrollViewComponent. So the first misconception in the issue is to imply "mouse must be over element" based on "mousemove". The events that actually give you this information are mouseenter/mouseleave.

That works just fine, but when I tested this on iPhone, I realized there isn't even a touchenter event yet, so this PR is adding that for the same abilities across plattforms.

I saw @jpauloruschel working with this and your last PR also changed the mousemove behaviour: ff00ed5

IMO the current behaviour is exactly how it should be, otherwise any developer who can only rely on mousemove events trying to implement e.g. a custom scroll element simply has no events to base the scrolling behaviour on.

Maybe I am completely wrong, any thoughts/feedback?

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

# Conflicts:
#	src/framework/input/element-input.js
Rewrite old TypeDefs
Fix not-definitely-assigned issue with _clickedEntities
@kungfooman
Copy link
Collaborator Author

Can anyone think of any objections to making _clickedEntities a definitely assigned property of ElementInput?

Right now in ElementInput#constructor:

if (platform.touch)
this._clickedEntities = {};

First it checks platform.touch and then later AppBase is calling ElementInput#attachSelectEvents:

if (this.elementInput)
this.elementInput.attachSelectEvents();

Which usually assigns ElementInput#_clickedEntities (just not in cases that a browser doesn't support XR):

attachSelectEvents() {
if (!this._selectEventsAttached && this._useXr && this.app && this.app.xr && this.app.xr.supported) {
if (!this._clickedEntities)
this._clickedEntities = {};
this._selectEventsAttached = true;
this.app.xr.on('start', this._onXrStart, this);
}
}

It would remove some lines of code and simplify the {}|undefined type to {}.

@kungfooman kungfooman changed the title Add touchenter event, add JSDoc, small refactor Add touchenter event, add JSDoc, small refactor, fix accessing not-definitely-assigned _clickedEntities Nov 13, 2022
@kungfooman
Copy link
Collaborator Author

I forked @mgawinKatana's project which handles now mouse/touch for testing this PR:

https://playcanvas.com/project/1007930/overview/elementmousemove-fix

https://launch.playcanvas.com/1587642

https://launch.playcanvas.com/1587642?use_local_engine=http://localhost/playcanvas-engine-pr/build/playcanvas.js

Is there a way to use_local_engine for 10.*.*.* or 192.168.*.* private networks? Then it would be testable on local touch devices connected via LAN/WLAN.

@yaustar
Copy link
Contributor

yaustar commented Nov 14, 2022

Is there a way to use_local_engine for 10...* or 192.168.. private networks? Then it would be testable on local touch devices connected via LAN/WLAN.

I'm afraid not, no.

The way I've been doing it is either overrides in the devtools or putting the engine file into the project and having it load after the engine.

Added ticket here: playcanvas/editor#925

@yaustar yaustar added the bug label Dec 15, 2022
# Conflicts:
#	src/framework/input/element-input.js
@kungfooman kungfooman changed the title Add touchenter event, add JSDoc, small refactor, fix accessing not-definitely-assigned _clickedEntities Add touchenter event, add JSDoc, small refactor Dec 15, 2022
Copy link
Contributor

@yaustar yaustar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to bubble these events to button like we do for touch leave https://github.com/playcanvas/engine/blob/main/src/framework/components/button/component.js#L297

}

this._fireEvent('touchmove', new ElementTouchEvent(event, oldTouchInfo.element, oldTouchInfo.camera, coords.x, coords.y, touch));
// And fire touchenter if we've entered the previously left touched element again
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we should fire touchenter if a user has clicked outside the element and moved into it as well. Seems odd to only fire it if we started in an element, leave and then re-enter

@GSterbrant GSterbrant added the open source contribution Issues related to contributions from people outside the PlayCanvas team label May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug open source contribution Issues related to contributions from people outside the PlayCanvas team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ElementComponent 'mousemove' event fires outside element when LMB is pressed
4 participants